Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove CMake/C++ references to cugraph-ops #4744

Open
wants to merge 4 commits into
base: branch-24.12
Choose a base branch
from
Open

Remove CMake/C++ references to cugraph-ops #4744

wants to merge 4 commits into from
+6 −453

Conversation

ChuckHastings
Copy link
Collaborator

Delete deprecated cugraph-ops functionality, clean up references to cugraph-ops.

@ChuckHastings ChuckHastings self-assigned this Nov 1, 2024
@ChuckHastings ChuckHastings added improvement Improvement / enhancement to an existing function breaking Breaking change and removed cuGraph CMake python labels Nov 1, 2024
@ChuckHastings ChuckHastings marked this pull request as ready for review November 4, 2024 04:25
@ChuckHastings ChuckHastings requested review from a team as code owners November 4, 2024 04:25
@jameslamb jameslamb self-requested a review November 4, 2024 14:42
jameslamb
jameslamb reviewed Nov 4, 2024
View reviewed changes
Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to remove ALL references to cugraph-ops here? If so, there are many more:

git grep -i 'cugraph.*ops'

For example, all the uses of those now-removed CMake options in build scripts.

CI configs (click me)

cugraph/.github/workflows/build.yaml

Lines 79 to 81 in 050d524

extra-repo: rapidsai/cugraph-ops
extra-repo-sha: branch-24.12
extra-repo-deploy-key: CUGRAPH_OPS_SSH_PRIVATE_DEPLOY_KEY

cugraph/.github/workflows/build.yaml

Lines 103 to 105 in 050d524

extra-repo: rapidsai/cugraph-ops
extra-repo-sha: branch-24.12
extra-repo-deploy-key: CUGRAPH_OPS_SSH_PRIVATE_DEPLOY_KEY

cugraph/.github/workflows/pr.yaml

Lines 140 to 142 in 050d524

extra-repo: rapidsai/cugraph-ops
extra-repo-sha: branch-24.12
extra-repo-deploy-key: CUGRAPH_OPS_SSH_PRIVATE_DEPLOY_KEY

cugraph/.github/workflows/pr.yaml

Lines 159 to 161 in 050d524

extra-repo: rapidsai/cugraph-ops
extra-repo-sha: branch-24.12
extra-repo-deploy-key: CUGRAPH_OPS_SSH_PRIVATE_DEPLOY_KEY

cugraph/.github/workflows/test.yaml

Line 26 in 050d524

symbol_exclusions: (cugraph::ops|hornet|void writeEdgeCountsKernel|void markUniqueOffsetsKernel)

build scripts (click me)

cugraph/build.sh

Line 46 in 050d524

--without_cugraphops

cugraph/build.sh

Line 110 in 050d524

BUILD_WITH_CUGRAPHOPS=ON

cugraph/build.sh

Line 271 in 050d524

-DUSE_CUGRAPH_OPS=${BUILD_WITH_CUGRAPHOPS} \

many more in build.sh

cugraph/ci/build_wheel_cugraph.sh

Line 31 in 050d524

export SKBUILD_CMAKE_ARGS="-DDETECT_CONDA_ENV=OFF;-DFIND_CUGRAPH_CPP=OFF;-DCPM_cugraph-ops_SOURCE=${GITHUB_WORKSPACE}/cugraph-ops/${EXTRA_CMAKE_ARGS}"

cugraph/ci/build_wheel_pylibcugraph.sh

Line 18 in 050d524

export SKBUILD_CMAKE_ARGS="-DDETECT_CONDA_ENV=OFF;-DFIND_CUGRAPH_CPP=OFF;-DCPM_cugraph-ops_SOURCE=${GITHUB_WORKSPACE}/cugraph-ops/${EXTRA_CMAKE_ARGS}"

dependency on 'libcugraphops' and 'pylibcugraphops' (click me)

cugraph/dependencies.yaml

Lines 23 to 24 in 050d524

# Deprecate pylibcugraphops
- depends_on_pylibcugraphops

cugraph/dependencies.yaml

Lines 921 to 944 in 050d524

depends_on_pylibcugraphops:
common:
- output_types: conda
packages:
- &pylibcugraphops_unsuffixed pylibcugraphops==24.12.*,>=0.0.0a0
- output_types: requirements
packages:
# pip recognizes the index as a global option for the requirements.txt file
- --extra-index-url=https://pypi.nvidia.com
- --extra-index-url=https://pypi.anaconda.org/rapidsai-wheels-nightly/simple
specific:
- output_types: [requirements, pyproject]
matrices:
- matrix:
cuda: "12.*"
cuda_suffixed: "true"
packages:
- pylibcugraphops-cu12==24.12.*,>=0.0.0a0
- matrix:
cuda: "11.*"
cuda_suffixed: "true"
packages:
- pylibcugraphops-cu11==24.12.*,>=0.0.0a0
- {matrix: null, packages: [*pylibcugraphops_unsuffixed]}

cugraph/dependencies.yaml

Lines 429 to 430 in 050d524

# Deprecate libcugraphops
- libcugraphops==24.12.*,>=0.0.0a0

cugraph/conda/recipes/libcugraph/meta.yaml

Line 72 in 050d524

- libcugraphops ={{ minor_version }}

and several more, see that git grep output

cpp/CMakeLists.txt Show resolved Hide resolved
@bdice
Copy link
Contributor

bdice commented Nov 4, 2024 •
edited
Loading

@jameslamb Since CI has already passed here I am inclined to just accept this as-is and then iteratively rip out more. Maybe we can limit this PR's scope to CMake / C++ removal, and then do the removals you suggested (mostly CI, build scripts, dependencies, etc.) in a follow-up PR. I started to look into this as well and noticed there are quite a few references in the GNN code, which is being split out, so there may be potentially duplicate work that we should defer until after that split.

@jameslamb
Copy link
Member

Ok yeah fair enough, let's stop here and do more in follow-ups.

@ChuckHastings ChuckHastings requested a review from a team as a code owner November 4, 2024 15:38
bdice
bdice reviewed Nov 4, 2024
View reviewed changes
python/cugraph/cugraph/tests/docs/test_doctests.py
@@ -186,11 +186,4 @@ def test_docstring(self, docstring):
f"{docstring.name}:\n{doctest_stdout.getvalue()}"
)
except AssertionError:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this is no longer using specialized logic in the exception clause, the "except: raise" pattern is unneeded. We can remove the try entirely.

@ChuckHastings ChuckHastings changed the title Remove references to cugraph-ops Remove CMake/C++ references to cugraph-ops Nov 4, 2024
seunghwak
seunghwak approved these changes Nov 5, 2024
View reviewed changes
Copy link
Contributor

@seunghwak seunghwak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice approved these changes

@jameslamb jameslamb jameslamb approved these changes

@seunghwak seunghwak seunghwak approved these changes

At least 1 approving review is required to merge this pull request.

Assignees

@ChuckHastings ChuckHastings

Labels
breaking Breaking change CMake cuGraph improvement Improvement / enhancement to an existing function python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ChuckHastings @bdice @jameslamb @seunghwak